Remove duplicate database code in provider logic#277
Remove duplicate database code in provider logic#277pcallewaert wants to merge 1 commit intomasterfrom
Conversation
pcallewaert
force-pushed
the
chore/remove-duplicate-code
branch
from
5f8f52e to
e57f3e5
Compare
pcallewaert
force-pushed
the
chore/remove-duplicate-code
branch
from
e57f3e5 to
13cc5e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
pkg/postgres/database.go:49
- The CreateDB function is missing logic to grant the role to the operator user before setting it as the database owner. This will fail on cloud providers (AWS RDS, Azure, GCP) where the operator user needs to be a member of the role to transfer ownership. The old cloud-specific implementations (aws.go, azure.go, gcp.go) all included this step before calling ALTER DATABASE OWNER. Without this, the ALTER DATABASE OWNER statement at line 40 will fail with a permission denied error.
func (c *pg) CreateDB(dbname, role string) error {
// Create database
err := c.execute(fmt.Sprintf(CREATE_DB, dbname))
if err != nil {
// eat DUPLICATE DATABASE ERROR
if !isPgError(err, "42P04") {
return err
}
}
err = c.execute(fmt.Sprintf(ALTER_DB_OWNER, dbname, role))
if err != nil {
return err
}
err = c.execute(fmt.Sprintf(GRANT_CREATE_SCHEMA, dbname, role))
if err != nil {
return err
}
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err = c.GrantRole(role, c.user) | ||
| if err != nil && !isPgError(err, "0LP01") { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The newly added logic to automatically grant the created role to the operator user may cause issues in some scenarios. This change modifies the behavior of CreateGroupRole even when cloud providers don't require it. If the operator role doesn't have CREATEROLE permission, this will fail. Additionally, silently ignoring the "already a member" error (0LP01) without logging could make debugging difficult when something goes wrong.
| err = c.GrantRole(role, c.user) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
The CreateUserRole function unconditionally grants the user role to the operator, but doesn't handle the "already a member" error (0LP01) like other similar code does. This will fail if the role membership already exists for some reason. The error handling should be consistent with CreateGroupRole at line 37 and DropRole at line 88.
pcallewaert
force-pushed
the
chore/remove-duplicate-code
branch
8 times, most recently
from
326426b to
fe875af
Compare
Replice terminate_backend with WITH (FORCE)
pcallewaert
force-pushed
the
chore/remove-duplicate-code
branch
from
fe875af to
0d5dcd2
Compare
1 participant
I noticed with the updated GCP logic that actually the logic for all cloud providers are very simular, and can be even applied on self-hosted postgres instances.
If this works properly, this means we can recommend to create a separate role specific for the operator with a select number of permissions.
Will close #110
WIP